Skip to content

Conversation

@erights
Copy link
Contributor

@erights erights commented Feb 4, 2025

Closes: #XXXX
Refs: #2002 #2008 #2113 #1739 Agoric/agoric-sdk#10952

Description

This PR adds a new M.containerHas(elementPatt, positiveBigint) matcher, and exported containerHasSplit function. This is motivated to support Agoric/agoric-sdk#10952 , which introduces a minimal form of want pattern in terms of M.containerHas.

Security Considerations

none

Scaling Considerations

Might help due to early termination of the split operations, which Agoric/agoric-sdk#10952 uses for AmountMath.isGTE.

Documentation Considerations

Already doc-documents M.containerHas in the types.js file for M. That's probably good enough for this PR. The interesting documentation will be explaining want patterns in Agoric/agoric-sdk#10952

Testing Considerations

Added tests for M.containerHas

Compatibility Considerations

The reason to postpone merging this PR until decisions are made on #2008 is that this PR will further expose rankOrder in the API, amplifying the danger that changing the string order will cause surprising observable changes.

Upgrade Considerations

This PR itself does not introduce any BREAKING changes or Upgrade issues.

  • Update NEWS.md for user-facing changes.

@erights erights self-assigned this Feb 4, 2025
@erights erights force-pushed the markm-m-has-matcher branch 2 times, most recently from f2ec8de to 143ba1f Compare February 4, 2025 20:44
@erights erights force-pushed the markm-m-has-matcher branch from 26dc7de to 9d8d743 Compare February 17, 2025 03:02
@erights erights force-pushed the markm-m-has-matcher branch 3 times, most recently from d4a686e to efb4c47 Compare March 14, 2025 20:48
@erights erights marked this pull request as ready for review March 14, 2025 21:13
@gibson042
Copy link
Member

Meta-commentary:

  • I think M.has is too general a name for the narrow behavior that this actually consists of. Some alternate possibilities:
    • M.{has,contains,includes}Element
    • M.{container,collection,bag}{Has,Contains,Includes}
    • M.{container,collection,bag}Satisfies
  • Are we comfortable for this limited API for composite want patterns? They are possible to create with M.and, but might be more convenient with a helper that accepts multiple (pattern, [count]) pairs (e.g., M.bagContains([[M.or(Sword, Staff)], [Dagger, 2n]]) rather than M.and(M.has(M.or(Sword, Staff)), M.has(Dagger, 2n)) for "at least a Sword or Staff, and also at least 2 Daggers").

@erights erights force-pushed the markm-m-has-matcher branch from abfc1f7 to 6998c50 Compare March 17, 2025 21:29
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @gibson042's suggestions on naming. Additionally, I wouldn't expect there to be many calls, so a long name is fine here.

@erights
Copy link
Contributor Author

erights commented Mar 17, 2025

I went with containerHas, so first bullet done.

@erights
Copy link
Contributor Author

erights commented Mar 17, 2025

As for the second, my whole approach with the pair of PRs is to get unstuck by adding minimal complexity needed to get enough expressiveness to cover our expected actual needs. We can always expand the patterns we accept in these roles later if

  • we find we need it
  • the need is worth paying the extra complexity cost.

@erights erights changed the title feat(patterns): M.has(el,n) to support want patterns feat(patterns): M.containerHas(el,n) to support want patterns Mar 17, 2025
@erights erights force-pushed the markm-m-has-matcher branch from 7aced6c to 8b3927b Compare March 17, 2025 22:30
@erights
Copy link
Contributor Author

erights commented Mar 17, 2025

I am ready to squash and merge. But in the OP (and at Show&Tell) I said

Actually merging this must happen only after we've decided either to move forward with #2008 or to give up on it. Once a decision is made, and even before it is acted on, then this PR can move forward. (Any decision to move forward or not with #2008 should also consider changing the default of the feature flag introduced by #2002 .)

I'd like to appeal this to my reviewers. The existence of this new feature will probably not cause the problem yet. The use of it by agoric-sdk will cause the problem. So I'd like to proceed so it gets in the next endo release, enabling a smoother experience of local testing and debugging of Agoric/agoric-sdk#10952

Reviewers, waddaya think? I'll wait a day for at least one yes and no objections.

@gibson042
Copy link
Member

We can always expand the patterns we accept in these roles later

AFAICT, such an expansion would be awkward unless it takes the form of an entirely new name, because e.g. M.containerHas([["foo"], ["bar"]]) is already valid in this PR ("has an element [["foo"], ["bar"]]") and therefore could not be reinterpreted to mean "has an element 'foo' and an element 'bar'". So if we anticipate potential introduction of the latter behavior, we should not burn its likely name on the former.

@erights
Copy link
Contributor Author

erights commented Mar 17, 2025

I was not thinking of either a new name nor a new pattern. Rather, using M.containerHas as a part of a larger pattern. This would require AmountMath to cope with more complicated patterns, though still hopefully a narrow subset of all patterns.

That's what I meant by "We can always expand the patterns we accept in these roles later". The "we accept" is more precisely "AmountMath accepts". M.containerHas already accepts any pattern as the elementPattern. I don't think the expressiveness of M needs to expand at all.

@turadg
Copy link
Member

turadg commented Mar 18, 2025

smoother experience of local testing and debugging of Agoric/agoric-sdk#10952

Given that locally one can have second checkouts of the two repos and yarn link them, I don't think the motivation given warrants expediting this into Endo master.

(And for remote testing Agoric/agoric-sdk#10952 is already using #endo-branch)

@erights
Copy link
Contributor Author

erights commented Mar 18, 2025

Thanks @turadg , I will stick with the original commitment. Since it is otherwise ready to go, I'm putting it in draft to protect me from myself ;)

@erights erights marked this pull request as draft March 18, 2025 01:48
@erights erights force-pushed the markm-m-has-matcher branch from 2250713 to f026934 Compare March 19, 2025 22:01
@erights erights marked this pull request as ready for review March 19, 2025 22:02
@erights
Copy link
Contributor Author

erights commented Mar 19, 2025

We have now decided to move forward with #2008 (thanks @mhofman @gibson042 ), so this PR can now be merged.

@erights erights enabled auto-merge (squash) March 19, 2025 22:03
@erights erights merged commit 01529a5 into master Mar 19, 2025
16 checks passed
@erights erights deleted the markm-m-has-matcher branch March 19, 2025 22:13
@erights
Copy link
Contributor Author

erights commented Mar 21, 2025

Meta-commentary:

  • I think M.has is too general a name for the narrow behavior that this actually consists of. Some alternate possibilities:

    • M.{has,contains,includes}Element
    • M.{container,collection,bag}{Has,Contains,Includes}
    • M.{container,collection,bag}Satisfies

@kriskowal suggests contains which I like too. But it doesn't fit with @gibson042 's suggested naming choices above. (FWIW, I still like has best.) @kriskowal also says he's willing to hold this upcoming release for this renaming.

So how do y'all @gibson042 @kriskowal @turadg @Chris-Hibbert @mhofman feel about contains vs containerHas? This is probably the last chance we have to painlessly rename this.

@gibson042
Copy link
Member

gibson042 commented Mar 21, 2025

I still think the name needs either a prefix or a suffix indicating its restriction to collection membership testing. Bare has seems like it should encompass CopyRecord property presence à la JavaScript Object.hasOwn and {Map,WeakMap}.prototype.has, and bare contains seems like it should encompass substring testing.

EDIT: Object.has is actually Object.hasOwn. Also referenced Map/WeakMap has(key).

@erights
Copy link
Contributor Author

erights commented Mar 21, 2025

Thanks @gibson042 .

Ok @kriskowal , we're going with containerHas. No need to revise the contributions from this PR.

@kriskowal
Copy link
Member

I still think the name needs either a prefix or a suffix indicating its restriction to collection membership testing. Bare has seems like it should encompass CopyRecord property presence à la JavaScript Object.has, and bare contains seems like it should encompass substring testing.

I agree that has is not a viable name for on the precedent of Object.has. I also agree that contains at least implies a multi-element argument if not specifically a string on the precedent of String.prototype.contains. I think includes is viable on the precedent of Array.prototype.includes, which has a single-element RHS and I believe is the analogous set-theory verb.

At this point, I’m not holding up the release on continued discussion, but do let me know if this is a compelling argument.

@gibson042
Copy link
Member

I think I could live with a single-element includes, especially if we had something pre-reserved for a multi-element generalization (should we decide to want that in the future). Would you have a proposal for that?

@kriskowal
Copy link
Member

I think I could live with a single-element includes, especially if we had something pre-reserved for a multi-element generalization (should we decide to want that in the future). Would you have a proposal for that?

For multi-element, I would favor contains, on the precedent of String.

@erights
Copy link
Contributor Author

erights commented Mar 21, 2025

The pattern we're talking about has a single subpattern (the elementPattern) and a positive nat count, but most certainly is not about a single member of the collection. In fact, if the count is >= 2 then is cannot be satisfied by a single member of the collection. That's why the earlier suggestion M.{has,contains,includes}Element made no sense to me. Whatever it is, it is plural in general, not singular.

Is it unclear what the pattern means?

@erights
Copy link
Contributor Author

erights commented Mar 21, 2025

I mention all this because includes makes the same mistake as M.{has,contains,includes}Element

@erights
Copy link
Contributor Author

erights commented Mar 21, 2025

In any case, I agree this is not about to converge, so we should go ahead with containerHas. We will regret it, but we can stand it.

@gibson042
Copy link
Member

My above use of "single-element" and "multi-element" should instead be read as "single-pattern" and "multi-pattern". As noted in the original comment, I do anticipate eventual introduction of multi-pattern composites to express more intricate wants.

@erights
Copy link
Contributor Author

erights commented Mar 23, 2025

Is there a string contains or array contains? I could not find them. Where?

@gibson042
Copy link
Member

ECMAScript String and Array/%TypedArray% includes were introduced as fallback choices for what was originally contains, the renaming (and deviation from Python et al.) being forced by MooTools as recorded in November 2014 (topic and conclusion). String.prototype.includes shipped in 2015 ES6 and {Array,%TypedArray%}.prototype.includes in 2016 ES7.

Map/Set/WeakMap/WeakSet has(key) was an initial method on the respective prototypes in 2015 ES6 (followed by Object.hasOwn as a static analogue of Object.prototype.hasOwnProperty in tc39/ecma262@077317b for the 2022 edition). And Set-only isSubsetOf/isSupersetOf were added (alongside other methods concerned with multiple elements) by Set Methods for JavaScript just last year.


But in Endo, a pattern function doesn't have the context of a prototype and so ought to indicate by name the domain of inputs for which it is applicable and corresponding predicate behavior over them—and they generally succeed at that! (splitArray and splitRecord being the outliers as indicated by our own internal use.) This one is tricky because a) it generalizes across CopyArray/CopySet/CopyBag but not CopyRecord or CopyMap, b) it is limited to a single pattern in a way that a plausible successor won't be, and c) due to the wide open domain of its first parameter, future introduction of a successor would almost certainly take the form of a new method rather than domain expansion. And in the world anticipated by point c, the name for the single-pattern method should not suggest behavior of the multi-pattern method, which is my biggest objection to an unqualified name here... even M.containerHas seems like it might be the method to reach for if one has a subset or subbag constraint and knows that such a method exists, so the latter would need a name that does an even better job of suggesting its general multi-pattern nature (which is made difficult by point a already generalizing from sets to bags, stretching otherwise obvious use of "superset" for such a purpose).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants